Don't replay commands from non-completed task #1750
Conversation
There was a problem hiding this comment.
Can you add an integration test that has a full history that replays successfully in the replayer, then confirm it continually succeeds in a replayer popping one event off the end each time?
Also, would like @Quinn-With-Two-Ns to look at this PR when he's available before we merge.
| func isTaskCompletedEvent(event *historypb.HistoryEvent) bool { | ||
| return event.GetEventType() == enumspb.EVENT_TYPE_WORKFLOW_TASK_COMPLETED | ||
| } |
There was a problem hiding this comment.
Just inline this single-use-single-line function, no need for a separate function
| @@ -1045,6 +1050,9 @@ ProcessEvents: | |||
| binaryChecksum := nextTask.binaryChecksum | |||
| nextTaskBuildId := nextTask.buildID | |||
| admittedUpdates := nextTask.admittedMsgs | |||
| if len(reorderedEvents) > 0 && isTaskCompletedEvent(reorderedEvents[0]) { | |||
There was a problem hiding this comment.
I didn't check the code, but I assume you have confirmed that for every event set, the initial index is where the task completed event lives? Is there ever a situation in the Go SDK besides partial task during worker replayer where reorderedEvents[0] is not task completed? Are there any concerns about some of this logic affecting non-replayer code paths?
| // We do not want to run non-determinism checks on a task start that | ||
| // doesn't have a corresponding completed task. | ||
| if completedTaskCommandIndex >= 0 { | ||
| replayCommands = replayCommands[:completedTaskCommandIndex] | ||
| } |
There was a problem hiding this comment.
Basically same question from above, is there ever a case this if evaluates to true when not using a replayer (i.e. in normal operation)?
What was changed
Skip any commands that aren't a part of a completed WFT (a.k.a. if WFT start is the last event)
Why?
Fix issue where we're hitting NDE when we shouldn't be.
Checklist
Closes Erroneous extra replay command when replaying mid-workflow tasks #1670
How was this tested:
Added new test